Add browser command as Playwright CLI proxy#644
Conversation
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
Adds a new `replayio browser` command that proxies to the bundled Playwright CLI, making it easy to control the browser for recording sessions. Features include session management, auto-upload on close, and generated Playwright test output. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
69eaaf8 to
835ff34
Compare
| ], | ||
| "homepage": "https://github.com/replayio/replay-cli/blob/main/packages/replayio/README.md", | ||
| "dependencies": { | ||
| "@playwright/cli": "latest", |
There was a problem hiding this comment.
let's not use latest like this, it's a pretty new package (2 weeks!) so I wouldn't be surprised if the interface changes somehow soon
| return sessions; | ||
| } | ||
|
|
||
| function escapeRegex(value: string): string { |
There was a problem hiding this comment.
| function escapeRegex(value: string): string { | |
| // replace with RegExp.escape once we can get away with node 24 as minimum supported version | |
| function escapeRegex(value: string): string { |
|
|
||
| function resolveFromPackage(): ResolvedCommand | null { | ||
| try { | ||
| const pkgPath = require.resolve("@playwright/cli/package.json"); |
There was a problem hiding this comment.
this should use resolve-from or require.resolve(pkgName, { paths: [__dirname] })
| } | ||
| } | ||
|
|
||
| async function uploadRecordingsInSubprocess( |
There was a problem hiding this comment.
why does this happen in a subprocess? u can just import things here and call an uploading util directly
| exitCode, | ||
| generatedTest, | ||
| }); | ||
| process.stdout.write(`${JSON.stringify(payload)}\n`); |
There was a problem hiding this comment.
The usage of process.stdout.write in this file feels quite excessive - most of those calls could easily be a normal-looking console.log
| const ENV_PATH_KEYS = [ | ||
| "REPLAYIO_PLAYWRIGHT_CLI_PATH", | ||
| "REPLAY_PLAYWRIGHT_CLI_PATH", | ||
| ]; |
There was a problem hiding this comment.
nit: I don't think you need to handle both. The new env variables are just using REPLAY_ prefix (the old format used RECORD_REPLAY_)
| "REPLAYIO_PLAYWRIGHT_CLI_PATH", | ||
| "REPLAY_PLAYWRIGHT_CLI_PATH", | ||
| ]; | ||
| const PLAYWRIGHT_EXECUTABLE_PATH_ENV = "PLAYWRIGHT_MCP_EXECUTABLE_PATH"; |
There was a problem hiding this comment.
nit: just inline this... having this constant doesn't benefit the code anyhow
|
|
||
| program | ||
| .command("browser") | ||
| .description("Proxy to the bundled Replay Playwright CLI") |
There was a problem hiding this comment.
nit: Technically, it's not bundled - it just gets installed alongside replayio given it's a regular dependency. But perhaps this wording makes sense here as a simplification
| const binRelative = | ||
| typeof binValue === "string" | ||
| ? binValue | ||
| : binValue?.["playwright-cli"] ?? binValue?.playwright ?? Object.values(binValue ?? {})[0]; |
There was a problem hiding this comment.
this really feels like redundant future-proofing. Literally all published versions of this package have this:
"bin": {
"playwright-cli": "./playwright-cli.js"
}You can't predict the future, just handle what's there right now
| return blocks; | ||
| } | ||
|
|
||
| function appendSessionSnippets(session: string, command: string, codeBlocks: string[]) { |
There was a problem hiding this comment.
it feels like making this session snippets file an append log would be better here, you could utilize appendFileSync instead of having to read the file, parse JSON and then write it back again. When reading the append log u can read it, split by new lines, parse individual lines and reduce the entries to what u want.
| function sleep(ms: number) { | ||
| return new Promise(resolve => setTimeout(resolve, ms)); | ||
| } |
There was a problem hiding this comment.
This is already available in node itself in timers/promises
| RECORD_ALL_CONTENT: process.env.RECORD_ALL_CONTENT || "1", | ||
| RECORD_REPLAY_METADATA: JSON.stringify({ | ||
| ...metadata, | ||
| browserSession: session, |
There was a problem hiding this comment.
this one feels redundant (I don't see it actually being used anywhere), especially given u already put session in the processGroupId
| for (let i = 0; i < args.length; i += 1) { | ||
| const arg = args[i]; | ||
| if (arg === PLAYWRIGHT_SESSION_FLAG) { | ||
| session = args[i + 1] ?? session; |
There was a problem hiding this comment.
this would interpret -s --cdp to set "--cdp" as the session name, right? that sounds like a bug
| const session = resolveBrowserSession(args); | ||
| const processGroupId = getSessionProcessGroupId(session); | ||
| const scopedRecordings = getRecordings(processGroupId); | ||
| const fallbackRecordings = getRecordings(); |
There was a problem hiding this comment.
why are those fallback recordings even computed here? it feel like overly defensive measure that muddies the true intent of the code. The processGroupId should "just work"
| context: SessionCloseContext, | ||
| options: { silent: boolean } | ||
| ) { | ||
| const recordings = await waitForSessionRecordings(context); |
There was a problem hiding this comment.
cant u trust the browser process to be closed at this point? if u can then polling implemented in this function would be redundant
Adds a new
replayio browsercommand that proxies to the bundled Playwright CLI, making it easy to control the browser for recording sessions. Features include session management, auto-upload on close, and generated Playwright test output.